@robertszczepanski - Is this change an enhancement or a bug?

This path appears to work fine in simulation, are we sure this isn't just an FPGA issue?

@Nitsirks - can u plz paste the waveform too showing the necessary signals?

@robertszczepanski - I am inclined to reject this in the current generation of HW as we don't want to take any enhancements at this stage. Plz review the waveform that Mike pasted above and let us know if you have any questions.

Thank you for your feedback @Nitsirks @bharatpillilli.

As pointed out as a reference in chipsalliance/Cores-VeeR-EL2#88, I was testing this using src/integration/test_suites/smoke_test_cg_wdt and I was able to reproduce a bug in the Verilator simulation.

From my understanding it's a bug caused by the usage of TEC_RV_ICG inside the rvdffpcie flip flop. This module samples an enable signal only on the falling edge of the clock. Due to such specified behavior, the first rising edge of the clock doesn't detect the asserted enable signal and din is propagated to dout 1 cycle later.

@Nitsirks can you please point me to the test that you've run in order to verify the described behavior in simulation and get the waveforms, so I can reproduce it?

It's the same test in VCS. Mike can correct me if I am wrong.

I ran smoke_test_clk_gating to verify the behavior.

I see why Verilator is failing now. The code in `TEC_RV_ICG is wrong for VERILATOR.

`ifdef VERILATOR
   always @(negedge CK) begin
      en_ff <= enable;
   end
`else
   always @(CK, enable) begin
      if(!CK)
        en_ff = enable;
   end
`endif

The VERILATOR code is modeling a negedge flop, while the real code is modeling a latch. In simulation the enable passes through the open latch to get captured by the first rising edge of the flop. In VERILATOR the enable can't propagate until the clock arrives, so it's 1 clock late.

Seems like Verilator is going to do best effort can than guarantee it :-)

I've removed different RTL for Verilator simulation and uncommented few additional tests that now successfully pass in CI. This resolves issues in simulation, waveform analysis confirms the correct behavior of PC after waking up from clock gating.

PR description has been accordingly updated.

@Nitsirks - if you can also confirm that we need these changes and they look good, I will approve/merge the PR.

I think allowing verilator to implement the latch fixed the issue Kiran filed.

Do we know if verilator runs take longer now? The quote above mentioned disabled performance optimizations around latches.

Worth rebasing for conflict resolution